Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement local and global variables handling for wasm simd #4056

Merged
merged 13 commits into from
Feb 14, 2025
Merged

Implement local and global variables handling for wasm simd #4056

merged 13 commits into from
Feb 14, 2025
+324 −94

Conversation

Zzzabiyaka
Copy link
Contributor

@Zzzabiyaka Zzzabiyaka commented Jan 27, 2025 •
edited
Loading

note:

I plan a bigger refactoring in a separate PR when we have all the spec tests passing so that I can easily check that there are no regressions from refactoring

@Zzzabiyaka Zzzabiyaka mentioned this pull request Jan 27, 2025
Closed
@Zzzabiyaka Zzzabiyaka force-pushed the makslit/local_global branch 8 times, most recently from d59ec3c to 8723135 Compare January 27, 2025 16:15
@Zzzabiyaka Zzzabiyaka mentioned this pull request Jan 27, 2025
Merged
@Zzzabiyaka Zzzabiyaka force-pushed the makslit/local_global branch 3 times, most recently from 362946f to b8c9245 Compare January 27, 2025 18:26
loganek
loganek reviewed Jan 27, 2025
View reviewed changes
core/iwasm/interpreter/wasm_interp_fast.c Outdated
@@ -3536,6 +3541,24 @@ wasm_interp_call_func_bytecode(WASMModuleInstance *module,
HANDLE_OP_END();
}

#if WASM_ENABLE_SIMDE != 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't seem right - it makes the assumption about SIMD implementation; if for some platforms there will be another library used to implement SIMD instructions, you'd have to remember to update this place (and the condition will become more and more complicated). We should attempt to refactor the code so the simple and universal condition WASM_ENABLE SIMD != 0 should be sufficient.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same comment for all the other similar conditions in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's tackle this separately and out of this PR

loganek
loganek reviewed Jan 27, 2025
View reviewed changes
core/iwasm/interpreter/wasm_interp_fast.c Outdated
addr = GET_OPERAND(uint32, I32, 0); \
frame_ip += 2; \
addr_ret = GET_OFFSET(); \
CHECK_MEMORY_OVERFLOW(4); \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't it check the address of the v128 destination? if so, should that be 4 or rather 16?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check this once more

@Zzzabiyaka Zzzabiyaka force-pushed the makslit/local_global branch 3 times, most recently from 5450cae to 36833e4 Compare January 28, 2025 13:49
@Zzzabiyaka Zzzabiyaka force-pushed the makslit/local_global branch 9 times, most recently from cc9b7ae to dbbbd0a Compare January 28, 2025 21:34
Zzzabiyaka
Zzzabiyaka commented Jan 28, 2025
View reviewed changes
core/iwasm/libraries/simde/simde.cmake
if (WAMR_BUILD_TARGET MATCHES "AARCH64.*" OR "ARM.*")
add_definitions (-DWASM_ENABLE_SIMDE=1)
endif ()
add_definitions (-DWASM_ENABLE_SIMDE=1)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should hopefully be gone after refactoring but keeping it here so far

@Zzzabiyaka Zzzabiyaka force-pushed the makslit/local_global branch from dbbbd0a to 8cae7cc Compare January 30, 2025 00:21
Zzzabiyaka and others added 12 commits February 12, 2025 16:28
@Zzzabiyaka
implement local and function calls for v128 in the fast interpreter
2ad14c8
@Zzzabiyaka
Fix splat opcodes, add V128 handling in preserve_referenced_local and…
4cb4067 
… reserve_block_ret
@Zzzabiyaka
implement globals
ab6c511
@Zzzabiyaka
Fix incorrect memory overflow values + SIMD ifdefs
817a746
@Zzzabiyaka
Fix load/load_splat macros
afbebad
@Zzzabiyaka
formatting commit
2b18750
@Zzzabiyaka
correct endif wasm loader
50e1972
@Zzzabiyaka @loganek
Update core/iwasm/interpreter/wasm_opcode.h
413c493 
Co-authored-by: Marcin Kolny <[email protected]>
@Zzzabiyaka @loganek
Update core/iwasm/interpreter/wasm_opcode.h
a5b9872 
Co-authored-by: Marcin Kolny <[email protected]>
@Zzzabiyaka @loganek
Update core/iwasm/interpreter/wasm_opcode.h
d4c3f04 
Co-authored-by: Marcin Kolny <[email protected]>
@Zzzabiyaka @loganek
Update core/iwasm/interpreter/wasm_opcode.h
fad514e 
Co-authored-by: Marcin Kolny <[email protected]>
@Zzzabiyaka
Fixes
7fe0f5a
@Zzzabiyaka
Fix load/store
993ff54 
Fix v128 load/store

style
@Zzzabiyaka
Copy link
Contributor Author

Zzzabiyaka commented Feb 12, 2025 •
edited
Loading

@lum1n0us could you help me with fixing the CI failure?

I see

  mkdir log
  cp $PWD/nuttx/.config log/dot-config
  cp  log
  tar -C apps/interpreters/wamr/wamr/tests/wamr-test-suites/workspace -cvzf log/report.tgz report
  shell: sh -e {0}
  env:
    LLVM_CACHE_SUFFIX: build-llvm_libraries_ex
    WASI_SDK_PATH: /opt/wasi-sdk
cp: cannot stat '/__w/wasm-micro-runtime/wasm-micro-runtime/nuttx/.config': No such file or directory

but I didn't really change anything about nuttx configs so not sure what would be my next steps

@Zzzabiyaka
Copy link
Contributor Author

Zzzabiyaka commented Feb 14, 2025 •
edited
Loading

Hi @lum1n0us @TianlongLiang @no1wudi

merging this is important for us to finish off the remaining items for WASM SIMD, could anyone take a look please?

I see #4082 is this going to be related to the failure we're seeing by any chance

loganek
loganek approved these changes Feb 14, 2025
View reviewed changes
Copy link
Collaborator

@loganek loganek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I'm merging it as this is a PR only to the dev branch; also the build failure is not related to the changes here.

@loganek loganek merged commit 93feee8 into bytecodealliance:dev/simd_for_interp Feb 14, 2025
370 of 383 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loganek loganek loganek approved these changes

@lum1n0us lum1n0us Awaiting requested review from lum1n0us lum1n0us is a code owner

@no1wudi no1wudi Awaiting requested review from no1wudi no1wudi is a code owner

@wenyongh wenyongh Awaiting requested review from wenyongh wenyongh is a code owner

@xujuntwt95329 xujuntwt95329 Awaiting requested review from xujuntwt95329 xujuntwt95329 is a code owner

@yamt yamt Awaiting requested review from yamt yamt is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Zzzabiyaka @loganek